Stop goroutines when closing a connection#39
Stop goroutines when closing a connection#39stolyaroleh wants to merge 5 commits intoBurntSushi:masterfrom stolyaroleh:close-channels
Conversation
- don't panic when calling Close() twice
|
|
||
| func (c *Conn) broadcastDone() { | ||
| select { | ||
| case <-c.done: |
There was a problem hiding this comment.
When readResponses hits an unrecoverable read error on line 435 and 463, it closes this channel by calling broadcastDone, thus signaling to all goroutines that they should shutdown. Since receive on a closed channel does not block, having:
select {
case <-c.done:
cleanup()
return
case ...:
...
}
in a goroutine will shut it down after broadcastDone.
I am doing a nonblocking select here to prevent c.done from being closed twice, since that will trigger a panic.
| // edits the generated code for the request you want to issue. | ||
| func (c *Conn) NewRequest(buf []byte, cookie *Cookie) { | ||
| select { | ||
| case <-c.done: |
There was a problem hiding this comment.
Could you explain this select statement in more detail?
There was a problem hiding this comment.
NewRequest waits until sendRequests processes current request. However, if an unrecoverable error happens, all goroutines, including sendRequests are stopped, and NewRequest will block forever. This select statement checks whether this connection was closed.
...however, now that you mention it, I can see a race condition here. The select statement should be:
select {
case c.reqChan<-&request:
case <-c.done:
return
case <-seq:
return
}
| @@ -366,9 +398,8 @@ func (c *Conn) writeBuffer(buf []byte) error { | |||
| if _, err := c.conn.Write(buf); err != nil { | |||
| Logger.Printf("A write error is unrecoverable: %s", err) | |||
There was a problem hiding this comment.
I wonder if broadcastDone should be called here, too. writeBuffer returns an error which is ignored. Sending another request will likely fail.
2 participants
Hello, @BurntSushi. I would like to use xgb in one of my projects. It's a service that runs in background and talks to the X server on behalf of all currently logged in users. Since it maintains a pool of X11 connections that can die at any moment (for example, when an interactive session stops on logout), I wanted to make sure that xgb will not panic/leave garbage behind when this happens.
This pull request attempts to fix #32, by introducing a
sync.WaitGroupand blocking inClose()until all 4 goroutines, spawned byNewConnNetdie. I tested it by creating and closing 100 000 connections - decreased RAM usage from at least 700 Mb to ~25 Mb.Additionally, I use
xgb.Conn.done(previouslyxgb.Conn.closing, achan struct{}) to broadcast that all goroutines need to stop (receive on closed channel does not block).Finally, I
broadcastDoneon unrecoverable read errors inreadResponsesinstead ofxgb.Conn.Close(). SincereadResponsescontains an infinite loop, and previously would justcontinueafterClose()(line 406, xgb.go), it would attempt toClose()again in the next iteration, causing a panic (closing an already closed channel).I don't have much experience with xgb or X11 in general and would greatly appreciate if you had a look at this, in case I missed something.